Skip to content

Remove unused stmcache.UnlimitedCache#2427

Merged
jackc merged 1 commit into
jackc:masterfrom
grongor:stmcache-use-unlimited-cache
Nov 9, 2025
Merged

Remove unused stmcache.UnlimitedCache#2427
jackc merged 1 commit into
jackc:masterfrom
grongor:stmcache-use-unlimited-cache

Conversation

@grongor

@grongor grongor commented Nov 8, 2025

Copy link
Copy Markdown
Contributor

There is a stmcache.Cache implementation UnlimitedCache that was never used. This PR changes that so that the UnlimitedCache is used when capacity in config is negative, and if the newly added config option is also set (for BC).

@jackc

jackc commented Nov 9, 2025

Copy link
Copy Markdown
Owner

To be honest I'd be more inclined to remove the unused stmtcache.UnlimitedCache rather than expose it. If I recall correctly, it was originally implemented as part of the internal functionality for preparing all statements used by a batch at once. But the final version didn't need it. I would consider it committed in error.

I also don't think encouraging an unlimited cache size is a good idea. I think an unlimited or extremely large cache size is more likely to cause problems such as OOM on the PostgreSQL server. While I was unable to find any definite benchmarks or rules, my understanding is that more than 1,000 prepared statements is asking for trouble. For reference, pgx's default max cache size is 512.

If you really want an unlimited cache size you can do that now by setting the cache size to math.MaxInt.

@grongor

grongor commented Nov 9, 2025

Copy link
Copy Markdown
Contributor Author

Hi, thanks for the quick response!

My use case was quite simple: I have an app with a limited number of queries (all generated by sqlc, no dynamic ones) and therefore I don't need to limit the cache size and I wanted to opt-in for a simpler cache implementation. My goal wasn't to really allow thousands of queries, but to simply don't bother with limiting them since it is a wasted time in my case. I think the lib users should have power to control stuff like that.

But your points are of course valid and if you chose to remove it instead, I'll live.

What if we add warning to the AllowUnlimitedCacheCapacity that mentions these possible issues to the user?

Let me know -> I'll either add the comment, or change the PR to remove it.

@jackc

jackc commented Nov 9, 2025

Copy link
Copy Markdown
Owner

What if we add warning to the AllowUnlimitedCacheCapacity that mentions these possible issues to the user?

I'm still against it. As mentioned above it would almost always be a mistake to set cache size above 1000. And if you really do need unlimited, you can set it to math.MaxInt.

Let me know -> I'll either add the comment, or change the PR to remove it.

Let's change it to remove UnlimitedCache.

@grongor grongor force-pushed the stmcache-use-unlimited-cache branch from 47c7b91 to ddc87c8 Compare November 9, 2025 13:33
@grongor grongor changed the title Use stmcache.UnlimitedCache when cap < 0 and if allowed Remove unused stmcache.UnlimitedCache Nov 9, 2025
@grongor

grongor commented Nov 9, 2025

Copy link
Copy Markdown
Contributor Author

✔️

@jackc jackc merged commit f2d5d44 into jackc:master Nov 9, 2025
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants